Refactor `PkH` fragment to support `Pk` #431

SarcasticNastik · 2022-06-09T17:52:19Z

As suggested in #427, this refactor helps to incorporate musig in the codebase.

SarcasticNastik · 2022-06-09T18:09:40Z

src/policy/compiler.rs

        }
        Concrete::Key(ref pk) => {
-            insert_wrap!(AstElemExt::terminal(Terminal::PkH(pk.to_pubkeyhash())));
+            insert_wrap!(AstElemExt::terminal(Terminal::PkH(


Ideally this insert_wrap should have been avoided (since we know Concrete::Key during compilation), but we have a change in internal representation for Terminal::PkH (found when one of tests fail in absence of this statement). As I understand, there can be 2 solutions:

Implement PartialEq for Terminal::PkH to only check equality in the field Pk::Hash. We can still access the Pk entry while having proper checks. Also, since we might require disambiguating a Terminal::PkH based on availability of Pk, Eq can check total equality by considering the first field too.

Keep both the compilations. Although I think this might give rise to ambiguity in the compilation and parsing miniscript from string can be an issue (internal representation of Terminal::PkH can't be inferred from a string representation as of now).

I'm open to any other suggestions too!

sanket1729 · 2022-06-09T18:35:47Z

@apoelstra , thinking about your comment here: bitcoin/bitcoin#24114 (comment). I am thinking it would make more sense to change Pkh(Pk::Hash) to Pk and add a new Terminal::RawPkh(Pk::Hash), just for parsing.

FromStr would not parse these. (until rawpkh is decided to supported upstream)
Compiler will always Pkh(Pk)
Parsing from the script would create a successful miniscript, but we will mark it insane. Meaning that it cannot be used in descriptors.

apoelstra · 2022-06-09T19:35:05Z

I like it. We could then also change semantic::Policy to have real public keys rather than hashes, which would simplify Simplicity interop.

sanket1729 · 2022-06-10T00:12:33Z

@SarcasticNastik, we need to modify this as follows:

Change Pkh(Pk::Hash) to Pkh(Pk)
Add a RawPkh node RawPkh(Pk::Hash) as commented

sanket1729

Will do another round tomorrow

src/interpreter/mod.rs

sanket1729 · 2022-06-13T10:19:17Z

src/miniscript/astelem.rs

            Terminal::PkK(ref p) => pred(ForEach(p)),
-            Terminal::PkH(ref p) => todo!("KeyHash should contain Pk"),
+            Terminal::PkH(ref p) => pred(ForEach(p)),
+            Terminal::RawPkH(ref p) => todo!("Should we require to iterate over the Hashes too?"),


We just ignore these. As these are not known keys.

src/miniscript/astelem.rs

sanket1729 · 2022-06-13T10:23:01Z

src/miniscript/astelem.rs

                            // alias: pk(K) = c:pk_k(K)
                            return write!(f, "pk({})", pk);
-                        } else if let Terminal::PkH(ref pkh) = sub.node {
+                        } else if let Terminal::RawPkH(ref pkh) = sub.node {


We can print this. But we should note in comments that RawPkH is currently unsupported in descriptor spec.

src/miniscript/astelem.rs

src/miniscript/decode.rs

src/policy/compiler.rs

src/miniscript/types/extra_props.rs

sanket1729

Mostly ACK, but the history is unclean. Can you remove the previous commits?

We can do the updating the miniscript tests stuff in a follow up PR.

sanket1729 · 2022-06-16T01:46:29Z

src/miniscript/mod.rs

-        ms_attributes_test("c:andor(ripemd160(6ad07d21fd5dfc646f0b30577045ce201616b9ba),pk_h(9fc5dbe5efdce10374a4dd4053c93af540211718),and_v(v:hash256(8a35d9ca92a48eaade6f53a64985e9e2afeb74dcf8acb4c3721e0dc7e4294b25),pk_h(dd100be7d9aea5721158ebde6d6a1fd8fff93bb1)))", "82012088a6146ad07d21fd5dfc646f0b30577045ce201616b9ba876482012088aa208a35d9ca92a48eaade6f53a64985e9e2afeb74dcf8acb4c3721e0dc7e4294b258876a914dd100be7d9aea5721158ebde6d6a1fd8fff93bb1886776a9149fc5dbe5efdce10374a4dd4053c93af5402117188868ac", true, false, true, 18, 3);
-        ms_attributes_test("c:andor(u:ripemd160(6ad07d21fd5dfc646f0b30577045ce201616b9ba),pk_h(20d637c1a6404d2227f3561fdbaff5a680dba648),or_i(pk_h(9652d86bedf43ad264362e6e6eba6eb764508127),pk_h(751e76e8199196d454941c45d1b3a323f1433bd6)))", "6382012088a6146ad07d21fd5dfc646f0b30577045ce201616b9ba87670068646376a9149652d86bedf43ad264362e6e6eba6eb764508127886776a914751e76e8199196d454941c45d1b3a323f1433bd688686776a91420d637c1a6404d2227f3561fdbaff5a680dba6488868ac", true, false, true, 23, 4);
-        ms_attributes_test("c:or_i(andor(c:pk_h(fcd35ddacad9f2d5be5e464639441c6065e6955d),pk_h(9652d86bedf43ad264362e6e6eba6eb764508127),pk_h(06afd46bcdfd22ef94ac122aa11f241244a37ecc)),pk_k(02d7924d4f7d43ea965a465ae3095ff41131e5946f3c85f79e44adbcf8e27e080e))", "6376a914fcd35ddacad9f2d5be5e464639441c6065e6955d88ac6476a91406afd46bcdfd22ef94ac122aa11f241244a37ecc886776a9149652d86bedf43ad264362e6e6eba6eb7645081278868672102d7924d4f7d43ea965a465ae3095ff41131e5946f3c85f79e44adbcf8e27e080e68ac", true, true, true, 17, 5);
+        ms_attributes_test("c:or_i(and_v(v:older(16),pk_h(03daed4f2be3a8bf278e70132fb0beb7522f570e144bf615c07e996d443dee8729)),pk_h(02352bbf4a4cdd12564f93fa332ce333301d9ad40271f8107181340aef25be59d5))", "6360b26976a91420d637c1a6404d2227f3561fdbaff5a680dba648886776a9148f9dff39a81ee4abcbad2ad8bafff090415a2be88868ac", true, true, true, 12, 3);


This is okay for this PR. As a followup, we should be using the same test vectors from https://github.com/sipa/miniscript/blob/fa7ea77b19d76738addce71430cb8ff50ee846b9/bitcoin/test/miniscript_tests.cpp#L461-L475 .

Now, we don't have to manually compute the hashes here

sanket1729 · 2022-06-16T02:01:09Z

src/lib.rs

-    Hash(&'a Pk::Hash),
-}
+/// Either a key or keyhash, but both contain Pk
+pub struct ForEach<'a, Pk: MiniscriptKey>(&'a Pk);


Note to reviewers. We deliberately do not iterate over RawPkH here. In a follow-up PR, we will make Miniscript::from_ast fail when we reach a RawPkh terminal.

We can update this whenever partial descriptors are supported upstream

In 09299b0:

Shouldn't we just drop the ForEach type entirely here?

I remember @sanket1729 and I discussed about removing this later on, but I don't see any reason why I can't do it here. I'm pushing another commit on top of 5cb2bcf to remove ForEach. If accepted, I can squash the commit anytime!

sanket1729 · 2022-06-16T02:10:45Z

Follow things that we can do after this (I will do some of them):

Make the sanity_checks fail when Terminal::RawPkH is used. Partial descriptors are not specified yet and are only used internally in the codebase.
- Make sure scripts when parsed from blockchain are correctly parsed using the RawPkH terminal. The interpreter should be able to produce partial descriptors. This might need some revisiting of things that we did in the PR. We can get a better idea as we started here
- Make sure that the user cannot create RawPkH descriptors directly. 1) Compiler 2) String or 3) Script. I think we are able to do this in this PR with correct usage of sanity_checks.
Update the translation APIs to remove the pkh altogether. The translation stuff is really useful when dealing with descriptors and we do not want users to reason about rawpkh as it contains information that they don't know about.
Change the semantic Policy Pk type from Key::Hash to Key
Clean up the iterator API so that everything is covered under pk.

Remove lifetime parameter concerning `PkH(Pk::Hash)` to `PkH(Pk)` refactor.

This change allows us to allow to refactor `Terminal::PkH(Pk::Hash)` to `Terminal::PkH(Pk)`.

sanket1729 · 2022-06-16T16:04:15Z

@apoelstra, can you have a high-level look at this? I would like to merge this first amongst all the other things that would be breaking first.

src/miniscript/decode.rs

src/miniscript/types/mod.rs

apoelstra · 2022-06-17T15:07:49Z

src/policy/mod.rs

-            Terminal::PkK(ref pk) => Semantic::KeyHash(pk.to_pubkeyhash()),
-            Terminal::PkH(ref pkh) => Semantic::KeyHash(pkh.clone()),
+            Terminal::PkK(ref pk) | Terminal::PkH(ref pk) => Semantic::KeyHash(pk.to_pubkeyhash()),
+            Terminal::RawPkH(ref pkh) => Semantic::KeyHash(pkh.clone()),


In 36ff0d2:

I guess this is for a future PR, but I'd kinda like to make semantic::Policy have only Pks, not Pk::Hashes, and then refuse to lift any scripts that had RawPkH in it. We could provide a transformer that took a Miniscript<Pk> to a Miniscript<Pk::Hash> maybe, converting RawPkHs to PkHs along the way, to get the old behavior.

Yup, the future TODOs are listed #431 (comment)

apoelstra · 2022-06-17T15:25:55Z

ack 36ff0d2 aside from the comments i made

The test-cases previously containing 40-byte hash for `pk_h` fragment have been replaced with 66-byte pubkey.

Since we disregard any `Pk::Hash` as a valid iterable key, only `Pk` is considered as such. The refactor reflects directly iterating over `Pk` (a valid Miniscript Key).

apoelstra

ACK 8d0cf06

sanket1729

ACK 8d0cf06. The last commit message should be Remove ForEach struct instead of Remove ForEach trait. But won't holdoff this over that.

Thanks for keeping with the reviews

This rule is weird and wrong in a number of ways. It was introduced in PR rust-bitcoin#97 c93bdef which was the massive PR that introduced the Ctx parameter. This particular rule wasn't commented on, but we can infer some things: * From the docs on the error and my vague memory, the problem was that we couldn't estimate satisfaction for a pubkeyhash fragment because we didn't know if the key was 33 or 65 bytes ahead of time. * Therefore, the code bans this in a legacy context, but not in bare contexts (in bare contexts there is a comment saying that bare fragments "can't contain miniscript because of standardness rules" so maybe we thought about this ... but that comment has never been true, so we thought wrong..). We should've banned the fragment in bare contexts as well. * Also, while the error is called MalleablePkH, none of this has anything to do with malleability! Later, in rust-bitcoin#431, we introduced the RawPkH fragment and put a pubkey into the PkH fragment. At that point, for PkH we always knew the pubkey and could just directly compute its size using the `is_uncompressed` accessor. The RawPkH fragment was a second-class citizen which didn't even have a string serialization. At this point we should have dropped the MalleablePkH rule. Still later, in rust-bitcoin#461, we introduced `ExtParams` and the ability to parse "insane" scripts in a more flexible way, including enabling the experimental `expr_raw_raw_pkh` fragment. We then gained an error `Analyzable::ContainsRawPkh` which conceptually overlaps with `ScriptContextError::MalleablePkH` but which isn't tied to any context, just whether the user has enabled this second-class feature.

…pport `Pk` 8d0cf061e4fcc3f7784116e43b2d2afb712cd066 Remove `ForEach` Trait (Aman Rojjha) 5cb2bcffae4f5720868138eadae7b98de0d0a9bc Refactor PkH to include key. (Aman Rojjha) 09299b072e8bbb703406192239f67ada37d53798 Refactor ForEach to contain only Pk. (Aman Rojjha) 701c9ccd2c30d0c91e7b7130312caa5daaaba2da Remove un-necessary lifetimes (Aman Rojjha) ca16bd86b9de807404bde882ef85801828d5b135 Fix formatter configuration errors with latest nightly (sanket1729) Pull request description: As suggested in #427, this refactor helps to incorporate musig in the codebase. ACKs for top commit: apoelstra: ACK 8d0cf061e4fcc3f7784116e43b2d2afb712cd066 sanket1729: ACK 8d0cf061e4fcc3f7784116e43b2d2afb712cd066. The last commit message should be `Remove ForEach struct` instead of `Remove ForEach trait`. But won't holdoff this over that. Tree-SHA512: 645974d9055e0458f64e214d95a73647420d46f05246be6809ae11dbfd8dacc05500c5e9f9f1b10b532b539b2d30883c014177a33e7666aea8ae383722910ff9

SarcasticNastik commented Jun 9, 2022

View reviewed changes

SarcasticNastik force-pushed the refactor/pk-hash branch 2 times, most recently from 4d63856 to e322327 Compare June 10, 2022 12:00

SarcasticNastik changed the title ~~Refactor PkH(Pk::Hash) -> PkH(Option<Pk>, Pk::Hash)~~ Refactor PkH fragment to support Pk Jun 10, 2022

SarcasticNastik force-pushed the refactor/pk-hash branch 3 times, most recently from 444b1fb to f170aaa Compare June 13, 2022 10:20

sanket1729 reviewed Jun 13, 2022

View reviewed changes

SarcasticNastik force-pushed the refactor/pk-hash branch 4 times, most recently from a3d2b36 to eb5b3d1 Compare June 15, 2022 23:57

SarcasticNastik marked this pull request as ready for review June 15, 2022 23:58

sanket1729 reviewed Jun 16, 2022

View reviewed changes

sanket1729 and others added 3 commits June 16, 2022 12:09

Fix formatter configuration errors with latest nightly

ca16bd8

Remove un-necessary lifetimes

701c9cc

Remove lifetime parameter concerning `PkH(Pk::Hash)` to `PkH(Pk)` refactor.

Refactor ForEach to contain only Pk.

09299b0

This change allows us to allow to refactor `Terminal::PkH(Pk::Hash)` to `Terminal::PkH(Pk)`.

SarcasticNastik force-pushed the refactor/pk-hash branch from eb5b3d1 to 36ff0d2 Compare June 16, 2022 06:39

sanket1729 mentioned this pull request Jun 17, 2022

Add Hash256 associated type #434

Merged

apoelstra reviewed Jun 17, 2022

View reviewed changes

src/miniscript/decode.rs Show resolved Hide resolved

apoelstra reviewed Jun 17, 2022

View reviewed changes

src/miniscript/types/mod.rs Outdated Show resolved Hide resolved

apoelstra reviewed Jun 17, 2022

View reviewed changes

Refactor PkH to include key.

5cb2bcf

The test-cases previously containing 40-byte hash for `pk_h` fragment have been replaced with 66-byte pubkey.

SarcasticNastik force-pushed the refactor/pk-hash branch from 36ff0d2 to 5cb2bcf Compare June 17, 2022 16:32

Remove ForEach Trait

8d0cf06

Since we disregard any `Pk::Hash` as a valid iterable key, only `Pk` is considered as such. The refactor reflects directly iterating over `Pk` (a valid Miniscript Key).

apoelstra approved these changes Jun 17, 2022

View reviewed changes

sanket1729 approved these changes Jun 17, 2022

View reviewed changes

sanket1729 merged commit 859534b into rust-bitcoin:master Jun 17, 2022

SarcasticNastik deleted the refactor/pk-hash branch June 17, 2022 21:13

apoelstra mentioned this pull request Oct 27, 2022

Add the "planning" module #481

Closed

Refactor PkH fragment to support Pk #431

Refactor PkH fragment to support Pk #431

Uh oh!

Conversation

SarcasticNastik commented Jun 9, 2022

Uh oh!

SarcasticNastik Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented Jun 9, 2022

Uh oh!

apoelstra commented Jun 9, 2022

Uh oh!

sanket1729 commented Jun 10, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sanket1729 Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sanket1729 Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

sanket1729 Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

apoelstra Jun 17, 2022

Choose a reason for hiding this comment

Uh oh!

SarcasticNastik Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented Jun 16, 2022

Uh oh!

sanket1729 commented Jun 16, 2022

Uh oh!

Uh oh!

Uh oh!

apoelstra Jun 17, 2022

Choose a reason for hiding this comment

Uh oh!

sanket1729 Jun 17, 2022

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Jun 17, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Refactor `PkH` fragment to support `Pk` #431

Refactor `PkH` fragment to support `Pk` #431

SarcasticNastik Jun 9, 2022 •

edited

Loading

SarcasticNastik Jun 17, 2022 •

edited

Loading